🤖 AI Code Review (two independent reviewers)

Reviewer responses: codex/ALL claude/ALL

Status: PASSED — No blocking issues found

All claims verified. Codex's stated mechanism for the sameEni finding is disproven by the base code (str = v == null ? '' : String(v) stringifies booleans distinctly, so true/false copies conflict correctly); Claude's milder variant of the same finding (true vs "true" coercion asymmetry with resolveEni's strict !== true check) is confirmed. Claude's L5 findings are confirmed against base: web/lib/CLAUDE.md:50 still lists only three critical types, and the stale "top 8 REAL types" comment block at web/app/page.tsx:346-349 is retained by the patch. No CRITICAL/MAJOR findings survive verification.

PR #259 Chair Review — fix(topology): complete live ENI reads and tighten owner resolution

Summary

This PR fixes three real defects: NAT gateway ENIs were never matched by InterfaceType (EC2 emits camelCase natGateway, the code tested nat_gateway), shared-VPC ENIs seen under multiple accounts were wrongly vetoed as ambiguous, and high-count ENIs could claim Core top-5 trend chips while being excluded from the fleet total they sit next to. It also removes the live page's RDS inventory reads (which could never produce a resolution — rds_proxy was already hard-coded incomplete) and promotes eni to a fully-paged critical type. Both panels independently found no CRITICAL or MAJOR issues; all surviving findings are MINOR.

Issues per lens

L2 — Code correctness

No CRITICAL/MAJOR. Both panels confirmed all new symbols resolve against BASE plus the patch (trendLegendGroups added and consumed; countsTowardFleetTotal/FLEET_TOTAL_EXCLUDED_TYPES pre-exist with eni in the excluded set; isDerivedTrendType import removals are safe), and the test arithmetic (21 reads / 25 pages / 17 healthy ledgers) is internally consistent.

• MINOR — sameEni boolean comparison via str() (web/lib/eni-owner.ts:56-58). Both models flagged this line, but with different mechanisms, and codex's is a false positive: str() is String(v), so boolean true/false copies stringify distinctly and correctly conflict — they cannot merge, contrary to codex's claim. The chair-verified residual issue is claude's variant: a boolean true and string "true" compare equal under str() and merge, while resolveEni (line 158) gates on strict requester_managed !== true, so the surviving copy's representation decides classification. Both copies come from the same JSONB projection, so this is unlikely in practice; tighten only if the merge predicate should exactly mirror the rule predicate.
• MINOR — OWNER_FIELDS omits the IP fields (one model flagged; verified). Two copies of one ENI that disagree on their IP set conflict only on overlapping IPs; an IP carried by one (possibly stale) copy still resolves from it. The seen.primary === primary check and the secondary_ip veto limit blast radius, but the comment "merge only when all of them agree" overstates the guarantee. Add the primary IP to OWNER_FIELDS or narrow the comment.
• MINOR — account-agnostic Lambda name lookups now more reachable (one model flagged; pre-existing). byRegionId keys owners by [region, resource_id] only; Lambda function names are not globally unique, so a multi-account scope can match a same-named function in the wrong account. The result is already marked inferred + correlation-blocked, and this PR only makes multi-account ENI input a supported case rather than introducing the gap.
• MINOR — eni as a critical type changes its failure mode (one model flagged; not a defect). A never-succeeded ENI ledger now fails loudly (invalid inventory response) instead of degrading quietly, which is fail-closed and matches the updated tests. In a large __all__ scope, up to 20 sequential pages are spent inside the shared 30-second/two-lane budget on a read that then counts as incomplete; a scope-aware skip with explicit disclosure would be cheaper and equally honest.
• MINOR — two existing critical-type test matrices not extended to eni (one model flagged): the consistency-marker rejection matrix and the "pages without paging display types" test (web/app/topology/page-ownership.test.tsx:355, :262) still cover only target_group/ecs_task/subnet.

L3 — Security / AWS mutation safety

No findings — both models agree. No AWS-mutating path, Terraform, IAM, or worker change; the AWS-facing surface shrinks (three read types dropped from the live loader). ADR-005's frozen mutation boundary and the ADR-015/ADR-023 exceptions are untouched. No hardcoded secrets; account IDs are documentation-range test fixtures. The ENI projection continues to keep private addresses only, and the new OWNER_FIELDS are all projected by the saved-graph read, avoiding a silent merge-on-absent-evidence trap. The contradictory-IP result changes from null to { reason: 'owner_ambiguous', eniId: null }; the sole consumer reads only reason, which is already in the vocabulary.

L4 — Observability / data-integration correctness

No CRITICAL/MAJOR. One model verified the substantive data claims in depth; the other concurred that incomplete-source handling stays fail-closed:

• The natGateway InterfaceType fix is correct — Steampipe's pinned aws plugin passes EC2's InterfaceType through verbatim, and natGateway is the one camelCase value in that vocabulary, so the old nat_gateway test never matched a real row.
• The new RDS owner_not_synced branch is correctly ordered after the gap('rds_cluster') incomplete check, so it can never claim "not synced" over an incomplete cluster read.
• The live/saved split is consistent: FLOW_ENRICHMENT in the graph store retains all six types, so RDS placement inference survives in the materialized graph while the live page passes RDS sources as unconditionally incomplete.
• MINOR (one model flagged): web/lib/ip-inventory.test.ts:41 fixture still uses InterfaceType: 'nat_gateway' — no behavioral bug (that path matches on description), but the fixture encodes a value AWS does not emit.

L5 — Docs/ADR consistency

No CRITICAL/MAJOR. The runbook, API reference, CHANGELOG (English and Korean, kept 1:1, amending the existing feature bullet in place per the CHANGELOG rule), and all four docs-site locales are mutually consistent with the code. No ADR needed touching.

• MINOR — stale critical-type list in web/lib/CLAUDE.md:50 (one model flagged; chair-verified): it still reads "target_group, ecs_task and subnet use bounded 20-page/500-row reads…" — the lone remaining copy of the sentence this PR updates in docs/api-reference.md and source-sync-observability.md.
• MINOR — contradicting comment left in place (one model flagged; chair-verified): web/app/page.tsx:346-349 still says "top 8 REAL types as toggle chips" and justifies hue-wrapping only via derived series; after this change the ninth chart slot can be the ENI series. The patch adds a correct one-line note but keeps the stale paragraph above it.
• MINOR (style): the English CHANGELOG sentence mixing a comma list with a semicolon splice ("…fleet totals, are never a Core Resources chip…; they are charted…") is hard to parse; split it.

Suggestions

1. Compare requester_managed with a boolean-aware check (or normalize it once at ingestion) and add the primary IP to OWNER_FIELDS, so the merge predicate exactly matches what the owner rules read.
2. Update web/lib/CLAUDE.md:50 and the page.tsx:346-349 comment block to reflect the four critical types and the fleet-counted (not "REAL") ranking — this repo treats those comments as behavioral spec.
3. Extend the consistency-marker and no-paging test matrices to include eni, and align the ip-inventory.test.ts NAT fixture to natGateway.
4. Consider a scope-aware skip (with an explicit "ENI ownership not attempted at this scope" disclosure) for very large multi-account scopes, where 20 sequential ENI pages consume the shared load budget only to count as incomplete.

Verdict

No CRITICAL or MAJOR findings from either panel survived verification; the one finding both models converged on was adopted only in its verified (MINOR) form, with codex's stated failure mechanism disproven against the base code. The image manifest lists no images, deletions, or omitted/unavailable entries.

IMAGE_COVERAGE: NOT_REQUIRED

Triggered by commit 1ebc9a30ce9f64c149ad60c7cc4d37272b920d4c · workflow: .github/workflows/pr-review.yml